Improve Arch Linux support and fix video export on Hyprland#484
Conversation
- Add pacman package build target for Arch Linux in electron-builder.json5 - Update build:linux script in package.json to include pacman target - Fix dialog window issues on Wayland/Hyprland: * Pass mainWindow reference to dialog.showSaveDialog and dialog.showOpenDialog in electron/ipc/handlers.ts * Required for proper dialog functionality on Wayland compositors * Previously dialogs opened without parent window attachment causing issues on Hyprland Changes ensure: - Correct video export on Arch Linux + Hyprland systems - Ability to install via pacman package manager - Improved compatibility with Wayland compositors
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDetect Wayland on Linux and add Electron command-line flags; make file dialogs attach to the main BrowserWindow when available; and add ChangesSingle change DAG
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f0483c65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 839-856: The dialog calls duplicate the options and pass
getMainWindow() directly which may return a stale/destroyed BrowserWindow;
create a small helper (e.g., buildDialogOptions or attachParentIfAlive) that
accepts the base options object and will attach a valid parent only when
getMainWindow() exists and !mainWindow.isDestroyed(), then use that helper for
dialog.showSaveDialog and the other dialog callers so you build the options once
(title, defaultPath, filters, properties) and only add a parent: mainWindow when
safe instead of passing getMainWindow() directly in
showSaveDialog/showOpenDialog call sites referenced here.
- Around line 362-364: The supportsWindowOpacity flag incorrectly treats Wayland
the same as non-Linux platforms; update the assignment of supportsWindowOpacity
(which currently references isWayland) to exclude Wayland so Linux always
follows the no-opacity codepath: remove the "|| isWayland" portion where
supportsWindowOpacity is computed, leaving isWayland as a separate boolean and
ensuring subsequent logic in the code that calls BrowserWindow.setOpacity(0) and
win.hide() (refer to supportsWindowOpacity, isWayland, BrowserWindow.setOpacity,
and win.hide) will run the Linux fallback path.
In `@electron/main.ts`:
- Line 41: Replace the incorrect Chromium feature name "PipeWire" used in
app.commandLine.appendSwitch("enable-features", "...") with the correct feature
"WebRTCPipeWireCapturer" so the call to app.commandLine.appendSwitch (in
electron/main.ts) enables WaylandWindowDrag and WebRTCPipeWireCapturer; update
the feature list string passed to app.commandLine.appendSwitch accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66c50af3-1976-48d7-9764-ef5b99972944
📒 Files selected for processing (4)
electron-builder.json5electron/ipc/handlers.tselectron/main.tspackage.json
- Add buildDialogOptions helper function to safely attach parent window only when valid and not destroyed - Update all dialog calls (save-exported-video, open-video-file-picker, save-project-file, load-project-file) to use the helper - Fix supportsWindowOpacity logic by removing || isWayland so Linux always follows no-opacity codepath - Change incorrect Chromium feature name 'PipeWire' to 'WebRTCPipeWireCapturer' in main.ts - Remove unused isWayland variable in handlers.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/main.ts (1)
33-43: wayland detection + pipewire flag — looks good 👍pipewire feature name is now correct (
WebRTCPipeWireCapturer), matching the previous round of feedback. detection viaXDG_SESSION_TYPE/WAYLAND_DISPLAYis the standard heuristic.two tiny nits, both optional:
process.env.WAYLAND_DISPLAY !== undefinedtreats an empty string as "wayland present". vanishingly rare in practice but a truthy check is slightly safer:- electron also supports
--ozone-platform-hint=auto, which lets chromium pick wayland when available and gracefully fall back to x11 otherwise. since you're already gating on a wayland check, hard-settingwaylandis fine — just flagging in case you want the softer landing for weird sessions (e.g. XWayland-only apps or broken compositors).optional tweak
- const isWayland = - process.env.XDG_SESSION_TYPE === "wayland" || process.env.WAYLAND_DISPLAY !== undefined; + const isWayland = + process.env.XDG_SESSION_TYPE === "wayland" || Boolean(process.env.WAYLAND_DISPLAY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.ts` around lines 33 - 43, Change the WAYLAND detection to treat an empty string as absent by replacing the strict undefined check with a truthiness check (e.g., use process.env.WAYLAND_DISPLAY instead of process.env.WAYLAND_DISPLAY !== undefined) in the isWayland calculation so isWayland is false for empty strings; optionally, if you prefer a softer fallback instead of forcing Wayland, switch the call to app.commandLine.appendSwitch("ozone-platform", "wayland") to use the hint form app.commandLine.appendSwitch("ozone-platform-hint", "auto") (or use that variant when you want Chromium to prefer Wayland but fall back to X11) while keeping the WebRTCPipeWireCapturer feature appendSwitch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@electron/main.ts`:
- Around line 33-43: Change the WAYLAND detection to treat an empty string as
absent by replacing the strict undefined check with a truthiness check (e.g.,
use process.env.WAYLAND_DISPLAY instead of process.env.WAYLAND_DISPLAY !==
undefined) in the isWayland calculation so isWayland is false for empty strings;
optionally, if you prefer a softer fallback instead of forcing Wayland, switch
the call to app.commandLine.appendSwitch("ozone-platform", "wayland") to use the
hint form app.commandLine.appendSwitch("ozone-platform-hint", "auto") (or use
that variant when you want Chromium to prefer Wayland but fall back to X11)
while keeping the WebRTCPipeWireCapturer feature appendSwitch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f43e5ca0-dc0b-4710-a618-29485c3f4ea7
📒 Files selected for processing (2)
electron/ipc/handlers.tselectron/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/handlers.ts
|
@psychosomat I will not be able to validate this/ test this. Waiting on someone who can help validate this before it can be merged |
No problem - I totally get that you already have enough of PR. Either way, I'd be happy to help. |
|
Is the installation via pacman/yay working for openscreen? The AUR version is not working (openscreen-git) when I tested it on hyprland + arch |
|
@utkarsh125 The AUR package was updated quite a while ago. To resolve this, download the .pacman file from the release page and install it using the command |
3 participants
Changes ensure:
Summary by CodeRabbit
New Features
Improvements